Skip to content

feat(State): Implement __delitem__ and pop methods for element removal #1856

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

PaveLuchkov
Copy link

feat(State): Implement __delitem__ and pop methods for element removal

Why?

The State class is designed to manage state and should mimic the behavior of a standard Python dictionary (dict). However, it previously lacked standard methods for element removal, such as del state[key] and state.pop(key).

This PR addresses this gap by making the State class more convenient and consistent with standard Python conventions.

What does this change do? (Implementation Details)

This PR introduces two standard dictionary-like removal methods to the State class:

  1. Implemented __delitem__(self, key):

    • Enables the use of the del state[key] syntax.
    • Correctly removes the key from both internal dictionaries (self._value and self._delta) to ensure state consistency.
    • Raises a KeyError if the key does not exist, fully replicating the behavior of a standard dict.
  2. Implemented pop(self, key, default=...):

    • Adds the standard .pop() method, which removes a key and returns its corresponding value.
    • Supports an optional default argument to prevent a KeyError when the key is not found.
    • The implementation elegantly reuses the existing __getitem__ and __delitem__.

How to use it? (Usage Examples)

# Initial state
state = State(
    value={'user:name': 'Alice', 'user:role': 'admin'}, 
    delta={'user:role': 'editor', 'app:theme': 'dark'}
)
# state.to_dict() would return: {'user:name': 'Alice', 'user:role': 'editor', 'app:theme': 'dark'}

# 1. Deleting with `del`
del state['app:theme']
# The key 'app:theme' is now removed from the state.

# 2. Deleting with pop (when the key exists)
# The __getitem__ method will retrieve the value from delta ('editor')
removed_role = state.pop('user:role')
print(removed_role)
# Output: 'editor'

# 3. Deleting with pop and a default value (when the key does not exist)
status = state.pop('user:status', 'active')
print(status)
# Output: 'active'
# No error is raised.

# 4. Attempting to delete a non-existent key will raise an error (just like in a dict)
try:
    del state['non_existent']
except KeyError:
    print("KeyError, as expected!")

Unit-tests have been passed
Code auto-format has been passed
Tested on my own and works perfectly. Small but useful addition for more flexible state management

@seanzhou1023
Copy link
Collaborator

thank you for contributing the fix :) However delete a key is not that simple like what you did in the PR. removing it from delta is not enough.
State is just a copy of the session state:

state: dict[str, Any] = Field(default_factory=dict)

You need to have some record to indicate the key is deleted, so that the actual session state will be updated accordingly. e.g. you also need to update below logic:

if event.actions and event.actions.state_delta:

@seanzhou1023 seanzhou1023 self-requested a review July 11, 2025 21:35
@PaveLuchkov
Copy link
Author

thank you for contributing the fix :) However delete a key is not that simple like what you did in the PR. removing it from delta is not enough. State is just a copy of the session state:

Thanks! Will see what I can do

@PaveLuchkov
Copy link
Author

@seanzhou1023
Made some changes. As you said only copy of state_delta have been deleting in previous commits.
Introduced a State._DELETED string marker to represent a deleted key in the state_delta. I chose a string to ensure proper serialization. The State class logic is updated to use this marker.
Updated relevant session services t process the _DELETED marker and perform the actual deletion from the persistent state.
Please let me know what you think. I'm ready to add tests for this new logic if needed.

@seanzhou1023 seanzhou1023 requested a review from DeanChensj July 18, 2025 01:15
@seanzhou1023
Copy link
Collaborator

@DeanChensj is the owner of session service, would better get his review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants